Skip to content

feat(workflow): align history propagation API with go-sdk#1825

Merged
WhitWaldo merged 14 commits into
dapr:masterfrom
nelson-parente:align-history-propagation-go-sdk
May 27, 2026
Merged

feat(workflow): align history propagation API with go-sdk#1825
WhitWaldo merged 14 commits into
dapr:masterfrom
nelson-parente:align-history-propagation-go-sdk

Conversation

@nelson-parente
Copy link
Copy Markdown
Contributor

@nelson-parente nelson-parente commented May 20, 2026

Summary

Aligns the workflow history propagation surface added in #1802 / #1818 with the canonical go-sdk shape — the same one python-sdk just adopted in python-sdk#1047. @cicoyle (durabletask-go author) flagged the divergence post-merge while building 1.18 quickstarts; this PR delivers what issue #1801 originally described.

This brings .NET in line for cross-SDK parity before 1.18 ships.

Three concrete gaps closed

1. Activity-level opt-in (was missing entirely)

Before this PR, PropagationScope only lived on ChildWorkflowTaskOptions — there was no way to propagate history when scheduling an activity. The go-sdk reference example uses exactly this pattern (SettlePayment activity with PropagateOwnHistory), so the cross-SDK quickstart literally couldn't be ported.

  • PropagationScope moved to base WorkflowTaskOptions.
  • WithHistoryPropagation() extension method available on both WorkflowTaskOptions and ChildWorkflowTaskOptions.
  • scheduleTaskAction.HistoryPropagationScope wired in WorkflowOrchestrationContext.CallActivityInternalAsync, matching the existing child-workflow path.

2. Read API replaced (was lossy filters + minimal events)

The old PropagatedHistoryEvent only kept (EventId, Kind, Timestamp) — input/output/failure payloads were thrown away by ConvertChunk, making the high-level audit/fraud-detection patterns from the Go/Python quickstarts impossible.

  • Removed: the old minimal PropagatedHistoryEvent and HistoryEventKind (clean break — 1.18 unreleased).
  • On PropagatedHistory: GetEntries(), FilterByWorkflowName(), TryGetLastWorkflowByName(), GetAppIds(), FilterByAppId(), FilterByInstanceId(). The FilterBy* names are kept for forward-compatibility (per review — propagated history may carry non-workflow entries in future runtimes), but now return the richer PropagatedHistoryEntry values rather than the old lossy event view.
  • New PropagatedHistoryEntry class (one per ancestor workflow) with TryGetLastActivityByName(), GetActivitiesByName(), TryGetLastChildWorkflowByName(), GetChildWorkflowsByName() — mirrors the GetLast*ByName rename merged in durabletask-go#105, adapted to the idiomatic .NET TryGet* pattern with [NotNullWhen(true)] out parameters.
  • New PropagatedHistoryActivityResult(Name, Started, Completed, Failed, Input, Output, FailureDetails) record — matches the Go SDK's ActivityResult struct and Python's ActivityResult dataclass.
  • New PropagatedHistoryChildWorkflowResult with the equivalent shape.
  • Both result records also expose a computed Status (PropagatedHistoryTaskStatus.Pending/Completed/Failed) projected from the flags (per review), for callers that prefer to switch on a single lifecycle value; the Started/Completed/Failed flags are retained for go-sdk/python-sdk field parity.
  • Lookups by name and app ID are case-insensitive, matching how WorkflowsFactory registers workflow/activity names and how the SDK matches app IDs elsewhere.

3. Event payload preserved internally

ConvertChunk now walks the raw HistoryEvents, pre-indexes TaskCompleted / TaskFailed (and the child-workflow counterparts) by TaskScheduledId in a single pass — so each scheduled activity or child resolves in O(1) — and produces fully-populated PropagatedHistoryActivityResult / PropagatedHistoryChildWorkflowResult records. SDK retries reuse TaskExecutionId, so we match on the scheduling event ID (matching Go/Python semantics). The proto HistoryEvent type is never exposed publicly — resolution happens at construction time inside Dapr.Workflow.

Usage example (matches the go-sdk reference)

```csharp
// Parent workflow propagates lineage to a child workflow and own-history to an activity
var fraudResult = await context.CallChildWorkflowAsync(
nameof(FraudDetection),
input: req,
options: new ChildWorkflowTaskOptions()
.WithHistoryPropagation(HistoryPropagationScope.Lineage));

await context.CallActivityAsync(
nameof(SettlePayment),
input: req,
options: new WorkflowTaskOptions()
.WithHistoryPropagation(HistoryPropagationScope.OwnHistory));

// Receive side — query specific events by name with TryGet*
var history = context.GetPropagatedHistory();
if (history is not null &&
history.TryGetLastWorkflowByName("MerchantCheckout", out var merchant) &&
merchant.TryGetLastActivityByName("ValidateMerchant", out var validate))
{
if (validate.Completed) { /* inspect validate.Output */ }
}
```

Non-goals

  • No example app in this PR. The cross-SDK quickstart lives in dapr/quickstarts#1310 and will be refreshed against this API once it merges.
  • No proto / runtime changes. All proto fields already exist (ScheduleTaskAction.historyPropagationScope field 6, CreateChildWorkflowAction.historyPropagationScope field 6); we just had to wire the activity path.

Breaking changes (1.18 unreleased)

The propagation API shipped in #1802/#1818 has not been released. This PR replaces it cleanly rather than carrying [Obsolete] shims for an API that has no users yet. Removed types: the old minimal PropagatedHistoryEvent and HistoryEventKind. (PropagatedHistoryEntry is reused as the new per-ancestor entry type with a richer shape; the PropagatedHistory.FilterByAppId/InstanceId/WorkflowName method names are retained but now return PropagatedHistoryEntry values.)

Test plan

  • WorkflowHistoryPropagationTests.cs rewritten end-to-end: activity resolution (completed/failed/pending/retried), child-workflow resolution, Status projection (incl. Failed-over-Completed precedence), history helpers (GetAppIds, TryGetLastWorkflowByName, FilterByWorkflowName, case-insensitive name/AppId lookups), scheduling option records (both base + derived), activity-level + child-workflow outbound action scope wiring.
  • HistoryPropagationWorkflowTests.cs (integration) updated to GetEntries().Count.
  • dotnet build + dotnet test to be verified by CI — couldn't build locally (no .NET 10 SDK on dev machine). Sweep confirmed no stale references to removed types in src/ or test/.

References

cc @cicoyle @WhitWaldo

Cassie (durabletask-go author) flagged the .NET surface for cross-SDK
divergence post-merge of dotnet-sdk#1802 / dapr#1818. This rewrites the
public history-propagation API to match the go-sdk shape — same one the
python-sdk just adopted (python-sdk#1047). Issue dotnet-sdk#1801 was
closed before her review; this PR delivers what the issue originally
described.

Three concrete gaps closed:

1. Activity-level opt-in (was missing entirely)
   - PropagationScope moved from ChildWorkflowTaskOptions to base
     WorkflowTaskOptions; ChildWorkflowTaskOptions inherits it.
   - WithHistoryPropagation() extension method added on the base record.
   - scheduleTaskAction.HistoryPropagationScope is now wired in
     WorkflowOrchestrationContext.CallActivityInternalAsync so activities
     can opt into propagation, matching CallChildWorkflowInternalAsync.
   - Without this, the Go SDK's reference example (SettlePayment activity
     using PropagateOwnHistory) literally cannot be ported to .NET.

2. Read API rewritten as high-level resolvers (was lossy FilterBy* + a
   PropagatedHistoryEvent record that dropped input/output/failure
   payloads)
   - PropagatedHistory.FilterByAppId/InstanceId/WorkflowName removed.
   - PropagatedHistory now exposes GetWorkflows(), GetWorkflowsByName(),
     GetLastWorkflowByName(), GetAppIds(), GetWorkflowsByAppId(),
     GetWorkflowsByInstanceId().
   - New WorkflowResult class with InstanceId/AppId/Name plus
     GetActivitiesByName(), GetLastActivityByName(),
     GetChildWorkflowsByName(), GetLastChildWorkflowByName() — mirrors
     durabletask-go's GetLastWorkflowByName / GetLastActivityByName /
     GetLastChildWorkflowByName renames from durabletask-go#105.
   - New ActivityResult record carries Name, Started, Completed, Failed,
     Input, Output, FailureDetails — matching the Go/Python equivalents
     so chain-of-custody patterns line up.
   - New ChildWorkflowResult record with the equivalent shape.

3. Event payload preserved internally (was discarded by ConvertChunk)
   - ConvertChunk in WorkflowOrchestrationContext now parses raw events,
     walks them to resolve TaskScheduled <-> TaskCompleted/Failed and
     ChildWorkflowInstanceCreated <-> ChildWorkflowInstanceCompleted/
     Failed by scheduleId, and produces fully-populated ActivityResult /
     ChildWorkflowResult instances. SDK retries reuse TaskExecutionId so
     matching is on scheduleId (matching Go/Python semantics).
   - Public API does not leak the proto HistoryEvent type — resolution
     happens at construction time inside Dapr.Workflow.

Additional surface additions:

- PropagationNotFoundException for missing-name lookups (mirrors
  Python's PropagationNotFoundError / Go's error returns).
- Static WorkflowHistory.PropagateLineage() / PropagateOwnHistory()
  factory helpers for go-sdk call-site parity.

Removed (clean break — 1.18 unreleased): PropagatedHistoryEntry,
PropagatedHistoryEvent, HistoryEventKind, FilterByAppId,
FilterByInstanceId, FilterByWorkflowName.

Tests:

- WorkflowHistoryPropagationTests.cs rewritten end-to-end to cover the
  new resolvers, query helpers, factory helpers, activity-level scope
  wiring, and child-workflow-level scope wiring.
- HistoryPropagationWorkflowTests.cs (integration) updated to use
  GetWorkflows().Count in place of Entries.Count.

Refs: dapr#1801, dapr/durabletask-go#105, dapr/go-sdk#823,
dapr/python-sdk#1047

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
@nelson-parente nelson-parente requested review from a team as code owners May 20, 2026 21:22
…ignment

- Document the `new`-hiding contract on ChildWorkflowTaskOptions
  .WithHistoryPropagation and add a regression test that asserts the
  returned type is ChildWorkflowTaskOptions (not the base record), so
  InstanceId survives the with-expression.
- Add the standard `()`, `(string)`, and `(string, Exception)` constructors
  on PropagationNotFoundException so callers can wrap inner exceptions.
- Alias StringValue alongside the existing Timestamp alias in
  WorkflowOrchestrationContext so the propagation helper signature stays
  consistent with the rest of the file.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Renames the test fixtures in GetPropagatedHistory_PreservesChunkOrder so the
variable order matches the documented oldest-first chunk ordering (index 0 is
the oldest ancestor, the last chunk is the immediate parent). No behavior change.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
protoc unwraps google.protobuf.StringValue to a plain string in the
generated C# (only the wire codec uses the wrapper). The
StringValueOrNull(StringValue?) helper added in this branch expected
the wrapper type, breaking the build with CS1503 at the three call
sites in ResolveActivity / ResolveChildWorkflow. Drop the helper and
pass the generated string fields straight through — they are already
nullable at runtime and ActivityResult/ChildWorkflowResult accept
string? for Input/Output.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Same StringValue mismatch as the production fix — protoc-generated
properties for google.protobuf.StringValue fields are plain string,
not the wrapper. Drop the new StringValue { Value = ... } wrappers
in the test helpers.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
cicoyle
cicoyle previously approved these changes May 21, 2026
@WhitWaldo
Copy link
Copy Markdown
Contributor

@nelson-parente I'm not inclined to accept the type name changes. For types that are exclusively used for workflow history propagation, it makes sense that they'd reflect that in the name. For example, there's no such thing as a WorkflowResult except in the context of history propagation - its more accurately described as a PropagatedHistoryEvent as originally named.

That said, completely agree about the activity propagation oversight.

I'll review this later this afternoon for specific notes, but I wanted to call this out for posterity.

Copy link
Copy Markdown
Contributor

@WhitWaldo WhitWaldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this PR together.

As I said in the earlier comment and repeat throughout, there's a distinction to me in terminology we use internally within the implementation of the project (in the gRPC protos, in the runtime) and the terminology we use in public-facing documentation. The protos refer to chunks and I've got no problem extending that to private methods, but most of this PR exposes public types with public-facing XML documentation - the names should be clear as to purpose and not introduce anything not present in publicly available Dapr documentation (like "chains" or "chunks"), especially when far more specific and particular names are already exposed in the implementation.

All of this work is scoped to history propagation - the names of these types should reflect this accordingly. It'd be one thing if you put them in a namespace like Dapr.Workflow.Abstractions.ProgagatedHistory and named them as such, but this isn't the long-standing Dapr .NET convention - most types are just left in the project root. So in this case, vague types like ActivityResult or ChildWorkflowResult just aren't clear. A developer shouldn't have to look at the XML documentation to understand that this isn't actually describing the returned result from a workflow or a child workflow - the name should be self-documenting, so I've proposed better alternatives like PropagatedHistoryActivityResult and PropagatedHistoryChildWorkflowResult.

The kind of event from the history was dropped in this implementation. Yes, it's not mentioned in #1801 but doesn't it provide some value to the user regarding what the event represents (e.g. activity started or completed)?

I certainly do appreciate you identifying the gap between supporting the activity propagation in addition to the existing workflow implementation.

Please do let me know if you have any follow-up questions.

Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistoryActivityResult.cs
Comment thread src/Dapr.Workflow.Abstractions/ChildWorkflowResult.cs Outdated
Comment thread src/Dapr.Workflow.Abstractions/WorkflowHistory.cs Outdated
Comment thread src/Dapr.Workflow.Abstractions/WorkflowResult.cs Outdated
Comment thread src/Dapr.Workflow.Abstractions/WorkflowResult.cs Outdated
Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistory.cs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protos describe the type of history event returned. Is this not something meaningful to provide to the user?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifecycle stage is captured by Started / Completed / Failed booleans on each result record — covers pending (Started && !Completed && !Failed) and matches go-sdk / python-sdk. HistoryEventKind removed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's premature to think that this might be the final state of the lifecycle functionality. A wall of flags in which one is only ever true seems cognitively confusing. I can anticipate developers preemptively asking if there's ever a situation in which more than one flag could ever be true at once - as this reflects a specific named lifecycle, no, in which case, it feels like a simpler effect here is to simply reflect the lifecycle enums of the original implementation. They needn't all be provided as only a subset are ever exposed here, but the idea is to think about how the SDK will be consumed most easily by developers, not just what's one of many ways to expose the concept.

Further, should additional lifecycle events be added, a wall of flag evaluations must be extended to properly rule out new lifecycle flags. But if the new enums aren't necessary, they can simply be ignored.

All told, let's use enums instead of flags.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing back here: Started/Completed/Failed mirrors the go-sdk ActivityResult struct and python-sdk ActivityResult dataclass, which is the whole point of this PR (cross-SDK parity Cassie flagged in #1801). Restoring a .NET-only enum would drop us back out of sync. The booleans are also not mutually exclusive over time — Started=true, Completed=false, Failed=false is the pending state — so an enum would actually lose information. Happy to switch if you want parity drift, but want to call that out explicitly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a PropagatedHistoryTaskStatus enum (Pending/Completed/Failed) with a computed Status property on both PropagatedHistoryActivityResult and PropagatedHistoryChildWorkflowResult, so callers can switch on a single value instead of reading the three flags — and the enum extends cleanly if the runtime adds lifecycle states later.

I kept the Started/Completed/Failed booleans alongside it: those are the field-level parity with the go-sdk ActivityResult/ChildWorkflowResult structs and the python-sdk dataclasses, and Status is just a projection of them (with Failed taking precedence over Completed). That keeps the .NET surface in sync with the other SDKs while giving developers the single enum to consume. 5987d9c

Copy link
Copy Markdown
Contributor

@WhitWaldo WhitWaldo May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design decisions behind the other SDKs isn't of much concern to me - those maintainers should do what is most appropriate per their own language conventions and express the functionality in ways that make it easier for their developers to consume their SDKs.

I weigh the perceived value of parity with other SDKs with the cost of expanding the public API and having to maintain two equivalent things going forward. I don't think a wall of flags is a developer-friendly way to communicate the state of something, so I appreciate you restoring the enum here.

Please modify the visibility of the flag fields to make them internal - should there ever be a need to make them publicly accessible, we can always introduce them later, but I'd rather this remain internal and not a part of the public API today.

bool Failed,
string? Input,
string? Output,
WorkflowTaskFailureDetails? FailureDetails);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No interest in reflecting what type of event was recorded here? It's in the protos.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Captured via Started / Completed / Failed booleans rather than a separate event-kind enum, matching go-sdk ActivityResult and python-sdk.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, don't care about making other SDKs. I care about consistency and ease of use in the .NET SDK itself. Workflow status is reflected by enum. Let's reflect activity status by enum as well.

bool Completed,
bool Failed,
string? Output,
WorkflowTaskFailureDetails? FailureDetails);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No interest in reflecting what type of event was recorded here? It's in the protos.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as PropagatedHistoryActivityResultStarted / Completed / Failed cover the lifecycle without a separate enum, matching go-sdk / python-sdk.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flags are more than fine to retain internally, but the point of the SDK Is to simplify development and lower the cognitive burden on developers.

It's a burden on developers to properly map which combination of flags represents what action, especially when we already produce a list of all possible workflow states as enums. It doesn't strike me as consistent to have activities reported as a wall of flags and workflows reported as an easy-to-understand enum.

Comment thread src/Dapr.Workflow/Worker/Internal/WorkflowOrchestrationContext.cs Outdated
Addresses Whit's review on dapr#1825:

- Rename ActivityResult -> PropagatedHistoryActivityResult
- Rename ChildWorkflowResult -> PropagatedHistoryChildWorkflowResult
- Rename WorkflowResult -> PropagatedHistoryEntry (primary constructor)
- Drop WorkflowHistory static class; callers pass HistoryPropagationScope directly
- Switch GetLast*ByName to TryGet*ByName + drop PropagationNotFoundException
- Drop chunk/chain terminology from public XML docs
- Surface malformed propagated event bytes via InvalidProtocolBufferException
  instead of silently skipping
- Update unit tests to new names and TryGet asserts

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Test names previously embedded the old type names (ActivityResult,
ChildWorkflowResult); rename to the more general Activity_/ChildWorkflow_
prefix to avoid confusion with the renamed public types.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Aligns the .NET workflow history propagation API with the Go/Python SDK shape by (1) enabling propagation scope on activity scheduling and (2) upgrading the read-side propagated history model from “minimal events” to typed activity/child-workflow results reconstructed from raw history events.

Changes:

  • Added PropagationScope to WorkflowTaskOptions and wired it into the activity scheduling (ScheduleTaskAction.HistoryPropagationScope) path.
  • Reworked propagated history domain types to expose workflow entries with resolved Activities and ChildWorkflows (including input/output/failure details), removing the older “event-kind” model.
  • Updated unit/integration tests to validate the new surface and behavior (including malformed raw-event handling).

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
test/Dapr.Workflow.Test/Worker/Internal/WorkflowHistoryPropagationTests.cs Rewritten unit tests for new propagated history shape, activity/child resolution, and outbound propagation scope wiring.
test/Dapr.IntegrationTest.Workflow/HistoryPropagationWorkflowTests.cs Updated integration test assertion to use the new GetWorkflows() API.
src/Dapr.Workflow/Worker/Internal/WorkflowOrchestrationContext.cs Wires activity propagation scope into ScheduleTaskAction; reconstructs propagated history entries from raw events.
src/Dapr.Workflow.Abstractions/WorkflowTaskOptions.cs Adds PropagationScope to base options + fluent WithHistoryPropagation; updates child options accordingly.
src/Dapr.Workflow.Abstractions/WorkflowContext.cs Updates GetPropagatedHistory() docs to point to the new lookup helpers.
src/Dapr.Workflow.Abstractions/PropagatedHistory.cs Replaces filter-based API with Get*/TryGet* navigation helpers and app/workflow/instance querying.
src/Dapr.Workflow.Abstractions/PropagatedHistoryEntry.cs Changes from a record of minimal events into a class representing a workflow entry with typed activity/child results + lookup helpers.
src/Dapr.Workflow.Abstractions/PropagatedHistoryActivityResult.cs Introduces typed activity result surfaced through propagated history.
src/Dapr.Workflow.Abstractions/PropagatedHistoryChildWorkflowResult.cs Introduces typed child-workflow result surfaced through propagated history.
src/Dapr.Workflow.Abstractions/PropagatedHistoryEvent.cs Removes the old minimal propagated event model.
src/Dapr.Workflow.Abstractions/HistoryEventKind.cs Removes the old public event-kind enum.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistory.cs Outdated
Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistory.cs Outdated
Comment on lines +80 to +83
ArgumentException.ThrowIfNullOrWhiteSpace(name);
return _workflows
.Where(w => string.Equals(w.Name, name, StringComparison.Ordinal))
.ToList();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely do this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already done in 25fe1d7 — all name lookups switched to OrdinalIgnoreCase.

Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistory.cs Outdated
Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistoryEntry.cs
Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistoryEntry.cs
Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistoryEntry.cs
Comment thread src/Dapr.Workflow.Abstractions/WorkflowContext.cs Outdated
Comment thread test/Dapr.Workflow.Test/Worker/Internal/WorkflowHistoryPropagationTests.cs Outdated
Comment thread src/Dapr.Workflow/Worker/Internal/WorkflowOrchestrationContext.cs
Copilot AI pushed a commit that referenced this pull request May 22, 2026
Addresses Whit's review on #1825:

- Rename ActivityResult -> PropagatedHistoryActivityResult
- Rename ChildWorkflowResult -> PropagatedHistoryChildWorkflowResult
- Rename WorkflowResult -> PropagatedHistoryEntry (primary constructor)
- Drop WorkflowHistory static class; callers pass HistoryPropagationScope directly
- Switch GetLast*ByName to TryGet*ByName + drop PropagationNotFoundException
- Drop chunk/chain terminology from public XML docs
- Surface malformed propagated event bytes via InvalidProtocolBufferException
  instead of silently skipping
- Update unit tests to new names and TryGet asserts

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
Copilot AI added a commit that referenced this pull request May 22, 2026
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/b8e98abe-cc48-40ba-ac78-f1cd82018a30

Co-authored-by: nelson-parente <20144601+nelson-parente@users.noreply.github.com>
…itively

Workflow / activity names register through WorkflowsFactory with
StringComparer.OrdinalIgnoreCase, and app IDs are matched case-insensitively
on the invocation path. Align the propagated history lookups (GetAppIds,
GetWorkflowsByName, TryGetLastWorkflowByName, GetWorkflowsByAppId,
GetActivitiesByName, TryGetLastActivityByName, GetChildWorkflowsByName,
TryGetLastChildWorkflowByName) with that contract so callers don't get
surprising misses or duplicate logical IDs that only differ by casing.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
ConvertChunk previously rescanned the full event list inside ResolveActivity
and ResolveChildWorkflow, making conversion O(n²) in the number of history
events. Pre-index TaskCompleted / TaskFailed by TaskScheduledId (and the
ChildWorkflowInstance counterparts) up front so each scheduled item resolves
in O(1).

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Copy link
Copy Markdown
Contributor

@WhitWaldo WhitWaldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more changes - not sure why you went and renamed several of the methods as the original names were clearer and more flexible to avoid future breaking changes. The new methods are more brittle and susceptible to needing to be modified should the scope of this implementation ever change (which it very well might in future runtime releases).

Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistory.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's premature to think that this might be the final state of the lifecycle functionality. A wall of flags in which one is only ever true seems cognitively confusing. I can anticipate developers preemptively asking if there's ever a situation in which more than one flag could ever be true at once - as this reflects a specific named lifecycle, no, in which case, it feels like a simpler effect here is to simply reflect the lifecycle enums of the original implementation. They needn't all be provided as only a subset are ever exposed here, but the idea is to think about how the SDK will be consumed most easily by developers, not just what's one of many ways to expose the concept.

Further, should additional lifecycle events be added, a wall of flag evaluations must be extended to properly rule out new lifecycle flags. But if the new enums aren't necessary, they can simply be ignored.

All told, let's use enums instead of flags.


/// <summary>
/// Returns a new <see cref="PropagatedHistory"/> containing only entries from the specified App ID.
/// Returns an ordered, deduplicated list of app IDs in the propagated chain.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know for a fact that propagated history will never include anything other than workflows? I don't know that and we have no roadmap indicating if this feature will evolve in the future. As this is a public method, I would rather retain the more generic "GetEntries()" name to keep our option open without introducing future breaking changes.

Comment on lines +80 to +83
ArgumentException.ThrowIfNullOrWhiteSpace(name);
return _workflows
.Where(w => string.Equals(w.Name, name, StringComparison.Ordinal))
.ToList();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely do this.

Comment thread src/Dapr.Workflow.Abstractions/PropagatedHistory.cs Outdated
public PropagatedHistory FilterByWorkflowName(string workflowName)
/// <param name="instanceId">The workflow instance ID to filter by.</param>
/// <returns>An empty list when no match is found.</returns>
public IReadOnlyList<PropagatedHistoryEntry> GetWorkflowsByInstanceId(string instanceId)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea as before - I don't know that this feature will always return only workflows. Today it might, but I'd like to leave our options open so we can avoid breaking changes. Please revert to "FilterByWorkflowName".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — GetWorkflowsByName()FilterByWorkflowName(), and for consistency GetWorkflowsByAppId()FilterByAppId() and GetWorkflowsByInstanceId()FilterByInstanceId(). TryGetLastWorkflowByName() stays (the TryGet pattern from the earlier round; "ByWorkflowName" lines up with FilterByWorkflowName). 5987d9c

nelson-parente and others added 4 commits May 25, 2026 11:14
The private field and ctor parameter on PropagatedHistory are now named
after the value type they hold (PropagatedHistoryEntry) rather than the
'workflows' role those entries play today. Public API surface is
unchanged.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
…Status enum

Addresses Whit's 2026-05-24 review.

Rename the PropagatedHistory query family to scope-neutral names so the public
surface need not change if propagated history ever carries non-workflow entries:
  GetWorkflows()             -> GetEntries()
  GetWorkflowsByName()       -> FilterByWorkflowName()
  GetWorkflowsByAppId()      -> FilterByAppId()
  GetWorkflowsByInstanceId() -> FilterByInstanceId()

Add PropagatedHistoryTaskStatus (Pending/Completed/Failed) and a computed
Status property on PropagatedHistoryActivityResult and
PropagatedHistoryChildWorkflowResult so callers can switch on a single value.
The Started/Completed/Failed flags are retained for go-sdk/python-sdk parity;
Status is a projection of them, with Failed taking precedence over Completed.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
FilterByAppId matches case-insensitively, so two entries whose app IDs
differ only in casing ("AppA" / "appa") both match a query for "APPA".
The de-duped GetAppIds list collapses to one, but the filter returns both;
assert two matches instead of one.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
/// </summary>
/// <param name="appId">The Dapr App ID to filter by.</param>
/// <returns>An empty list when no match is found.</returns>
public IReadOnlyList<PropagatedHistoryEntry> FilterByAppId(string appId)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im very adamant to have consistency across sdks. Can we ensure func names are consistent and not use filter in the func name?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cicoyle I'll address this in a separate PR so I can avoid playing any more telephone here :)

@WhitWaldo WhitWaldo merged commit 5ad31e6 into dapr:master May 27, 2026
444 of 447 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for workflow history propagation

4 participants